Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BigQuery: Add to_standard_sql() method to SchemaField #8880

Merged
merged 3 commits into from
Aug 5, 2019

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Aug 1, 2019

Closes #8494.

This PR adds a method to convert a legacy table field instance to the new StandardSqlField type.

How to test

Verify that the types are correctly mapped from the legacy to standard SQL, and that the new method does what it is supposed to do.

@plamut plamut added the api: bigquery Issues related to the BigQuery API. label Aug 1, 2019
@plamut plamut requested a review from a team August 1, 2019 15:24
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 1, 2019

# NOTE: No need to also handle the "ARRAY" composed type, the latter
# does not exist in legacy SQL types.
if sql_type.type_kind == types.StandardSqlDataType.STRUCT: # noqa: E721
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

False positive here, flake8 thinks that is about comparing types and wants to enforce isinstance() - but that would not work, as we actually compare integers here.

bigquery/google/cloud/bigquery/schema.py Show resolved Hide resolved
bigquery/google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
@plamut
Copy link
Contributor Author

plamut commented Aug 2, 2019

Support for standard SQL type names and simple arrays added. Still need to add support for arrays containing STRUCTs, do not merge yet.

@plamut plamut added the needs work This is a pull request that needs a little love. label Aug 2, 2019
@plamut plamut removed the needs work This is a pull request that needs a little love. label Aug 2, 2019
@plamut plamut requested a review from tswast August 2, 2019 17:52
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for adding tests for arrays of structs.

This reminds me that I wrote some similar code recently at

Would it make any sense to share any of that code here?

@plamut
Copy link
Contributor Author

plamut commented Aug 5, 2019

@tswast You mean sharing the conversion logic or the test for it?

Since the "target results" differ, there are differences between the two implementations, and they are both reasonably short, too, thus we might not actually gain that much by refactoring.

(but of course, if you see something "obvious" that would represent a good opportunity, we can add this improvement in another PR)

@plamut plamut merged commit 0ce1ca5 into googleapis:master Aug 5, 2019
@plamut plamut deleted the iss-8494 branch August 5, 2019 08:39
@tswast
Copy link
Contributor

tswast commented Aug 6, 2019

Agreed. Not obvious to me right now how to share code without it getting more complex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigQuery: SchemaField.to_standard_sql and SchemaField.from_standard_sql methods
3 participants